fix(kafka): Fix internal docker connection #2894
Conversation
Co-authored-by: Manuel de la Peña <[email protected]>
Co-authored-by: Manuel de la Peña <[email protected]>
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
mdelapenya
changed the title
modules/kafka/options.go
Outdated
| func defaultOptions() options { | ||
| return options{ | ||
| Listeners: make([]KafkaListener, 0), | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion: I suspect this isn't needed.
| @@ -525,7 +525,8 @@ func TestRedpandaListener_NoNetwork(t *testing.T) { | |||
| func TestRedpandaBootstrapConfig(t *testing.T) { | |||
| ctx := context.Background() | |||
|
|
|||
| container, err := redpanda.RunContainer(ctx, | |||
| container, err := redpanda.Run(ctx, | |||
There was a problem hiding this comment.
suggestion: this should be a separate PR as its a different module.
|
@stevenh I addressed the majority of your comments: all except those related to the |
| // Consumer represents a Sarama consumer group consumer | ||
| type TestConsumer struct { | ||
| t *testing.T | ||
| ready chan bool |
There was a problem hiding this comment.
suggestion: its idiomatic to use a struct{} for this pattern
| ready chan bool | |
| ready chan struct{} |
|
|
||
| return TestConsumer{ | ||
| t: t, | ||
| ready: make(chan bool), |
There was a problem hiding this comment.
| ready: make(chan bool), | |
| ready: make(chan struct{}), |
| @@ -38,6 +38,12 @@ type KafkaContainer struct { | |||
| ClusterID string | |||
| } | |||
|
|
|||
| type Listener struct { | |||
There was a problem hiding this comment.
suggestion: Add doc comment.
| brokers, err := kafkaContainer.Brokers(context.TODO()) | ||
| require.NoError(t, err) | ||
|
|
||
| // err = createTopics(brokers, []string{topic_in, topic_out}) |
There was a problem hiding this comment.
suggestion: remove commented out code.
| if ctx.Err() != nil { | ||
| return | ||
| } | ||
| consumer.ready = make(chan bool) |
There was a problem hiding this comment.
bug: this looks racy to me as its overwriting the read which is already set in the call to NewTestConsumer
| t.Log("Sarama consumer up and running!...") | ||
|
|
||
| cancel() | ||
| wg.Wait() |
There was a problem hiding this comment.
question: is the waitgroup and ready doing the same thing?
| if errors.Is(err, sarama.ErrClosedConsumerGroup) { | ||
| return | ||
| } | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
bug: you can't fail a test in a goroutine.
|
|
||
| func validateListeners(listeners ...Listener) error { | ||
| // Trim | ||
| for i := 0; i < len(listeners); i++ { |
There was a problem hiding this comment.
question: doesn't the caller needed the update listeners to work correctly?
| listeners[i].Host = strings.Trim(listeners[i].Host, " ") | ||
| listeners[i].Port = strings.Trim(listeners[i].Port, " ") |
There was a problem hiding this comment.
question: is this just space char or should this use strings.TrimSpace?
| listeners[i].Name = strings.ToUpper(strings.Trim(listeners[i].Name, " ")) | ||
| listeners[i].Host = strings.Trim(listeners[i].Host, " ") | ||
| listeners[i].Port = strings.Trim(listeners[i].Port, " ") | ||
| } |
There was a problem hiding this comment.
question: as there's no constructor hence these fields could be blank do we need to test for that?
What does this PR do?
Follow-up of #2490 in order to be able to push commits and make progress.
Why is it important?
Give importance to @catinapoke's work in #2490, keeping the commits (and authorship), and doing the toil tasks for them. This way we can push to this feature branch (the original PR is using the main branch, so we cannot update it)
Related issues